Skip to content

ARROW-1875: [Java] Write 64-bit ints as strings in integration test JSON files#4977

Closed
tianchen92 wants to merge 1 commit into
apache:integration_ints_as_stringsfrom
tianchen92:ARROW-1875
Closed

ARROW-1875: [Java] Write 64-bit ints as strings in integration test JSON files#4977
tianchen92 wants to merge 1 commit into
apache:integration_ints_as_stringsfrom
tianchen92:ARROW-1875

Conversation

@tianchen92

Copy link
Copy Markdown
Contributor

Related to ARROW-1875.
This is Java side implementation.

@codecov-io

Copy link
Copy Markdown

Codecov Report

Merging #4977 into integration_ints_as_strings will increase coverage by 2.13%.
The diff coverage is n/a.

Impacted file tree graph

@@                       Coverage Diff                       @@
##           integration_ints_as_strings    #4977      +/-   ##
===============================================================
+ Coverage                        87.57%   89.71%   +2.13%     
===============================================================
  Files                             1002      667     -335     
  Lines                           142651    98283   -44368     
  Branches                          1418        0    -1418     
===============================================================
- Hits                            124929    88176   -36753     
+ Misses                           17360    10107    -7253     
+ Partials                           362        0     -362
Impacted Files Coverage Δ
cpp/src/arrow/json/converter.cc 90.05% <0%> (-1.76%) ⬇️
cpp/src/arrow/json/chunked-builder.cc 79.91% <0%> (-1.68%) ⬇️
r/src/recordbatch.cpp
r/R/Table.R
js/src/util/fn.ts
go/arrow/array/bufferbuilder.go
r/src/symbols.cpp
rust/datafusion/src/execution/projection.rs
rust/datafusion/src/execution/filter.rs
rust/arrow/src/csv/writer.rs
... and 328 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update dc2c054...20cc581. Read the comment docs.

@wesm

wesm commented Aug 1, 2019

Copy link
Copy Markdown
Member

I'm surprised that the integration tests pass. Is RapidJSON loose about parsing strings to numbers?

@tianchen92

Copy link
Copy Markdown
Contributor Author

Also surprised me, I thought it would only pass if all the implementations work is done. Since it not break travis, could we just merge this into master or hold it in integration branch?

@kou

kou commented Aug 3, 2019

Copy link
Copy Markdown
Member

Why does this pull request target to https://github.com/apache/arrow/tree/integration_ints_as_strings branch instead of master?
Should we use the branch? Can we remove the branch?

@emkornfield

Copy link
Copy Markdown
Contributor

@kou we thought this change would need coordination between multiple languages. It appears it might not. @tianchen92 do you want to try to make the pull request against master and see if the tests still pass?

@tianchen92

Copy link
Copy Markdown
Contributor Author

@kou we thought this change would need coordination between multiple languages. It appears it might not. @tianchen92 do you want to try to make the pull request against master and see if the tests still pass?

I think i will work fine in master, I just opened a new PR #5002, close this one and will reopen if needed.

@tianchen92 tianchen92 closed this Aug 4, 2019
@kou

kou commented Aug 4, 2019

Copy link
Copy Markdown
Member

@emkornfield Thanks for the answer. There is no problem if we do this intentionally. I thought that this is caused by error. Because we don't use a branch like this.
Sorry for missing a discussion for this branch usage.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants